[Enhancement] use stream load async return to optimize the performance under high concurrency #53144
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
July 12, 2025 12:04
…e better http performance * fix some bug for partition * fix for thrift * fix the thrift exit bug * Revert "[feat](http): use async reply to provide better http performance * ensure free order * [Fix](stream-load) Fix stream load stuck under high concurrency (apache#36772) When the concurrency of streamload exceeds the number of threads in the remote scanner, streamload may get stuck. The reason is that the libevent thread blocks and waits for streamload to complete, and when there is no intersection between the tasks handled by the scanner thread and the libevent thread, it gets stuck. The solution is to convert the synchronous waiting tasks of libevent into asynchronous execution by using callbacks in the streamload executor thread. See merge request: !740" Revert commit d9e74efa762c8161a5ca3df4290bbd0ab896f1ef See merge request: !745" Revert commit 396cb2ec7e0b1a21bc0d7424c627f0d9321884bc
* fix missing ; * fix stream load block See merge request: !855
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
gavinchou
reviewed
Jul 14, 2025
| } | ||
|
|
||
| int StreamLoadAction::on_header(HttpRequest* req) { | ||
| req->mark_send_reply(); |
Contributor
Author
There was a problem hiding this comment.
只有 StreamLoad 请求，另外只是提交 reply 是 async 的，客户端感知仍然是同步的。
gavinchou
reviewed
Jul 14, 2025
| } | ||
|
|
||
| private: | ||
| SendReplyType _send_reply_type = REPLY_SYNC; |
Contributor
There was a problem hiding this comment.
这个初始值是不是没啥用 因为 on_header 把它改写了
Contributor
Author
There was a problem hiding this comment.
还有其他 HttpAction 会用到，他们需要默认是 SYNC 的。
gavinchou
previously approved these changes
Jul 14, 2025
github-actions
bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
Contributor
|
PR approved by anyone and no changes requested. |
Contributor
|
run buildall |
github-actions
bot
removed
the
approved
gavinchou
previously approved these changes
Jul 14, 2025
Contributor
Author
|
run buildall |
github-actions
bot
added
the
approved
Contributor
|
PR approved by at least one committer and no changes requested. |
added 4 commits
July 15, 2025 10:55
…e better http performance * fix some bug for partition * fix for thrift * fix the thrift exit bug * Revert "[feat](http): use async reply to provide better http performance * ensure free order * [Fix](stream-load) Fix stream load stuck under high concurrency (apache#36772) When the concurrency of streamload exceeds the number of threads in the remote scanner, streamload may get stuck. The reason is that the libevent thread blocks and waits for streamload to complete, and when there is no intersection between the tasks handled by the scanner thread and the libevent thread, it gets stuck. The solution is to convert the synchronous waiting tasks of libevent into asynchronous execution by using callbacks in the streamload executor thread. See merge request: !740" Revert commit d9e74efa762c8161a5ca3df4290bbd0ab896f1ef See merge request: !745" Revert commit 396cb2ec7e0b1a21bc0d7424c627f0d9321884bc
* fix missing ; * fix stream load block See merge request: !855
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 31950 ms |
TPC-DS: Total hot run time: 173549 ms |
ClickBench: Total hot run time: 26.99 s |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 32001 ms |
TPC-DS: Total hot run time: 173277 ms |
ClickBench: Total hot run time: 27.01 s |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
dataroaring
reviewed
Jan 5, 2026
Contributor
dataroaring
left a comment
dataroaring
left a comment
There was a problem hiding this comment.
Code Review: Stream Load Async Return Optimization
Thanks for this PR! The optimization concept is sound - making HTTP replies asynchronous should improve thread utilization under high concurrency. I've traced through the synchronization logic and verified it handles race conditions correctly.
Critical Issues (Please Fix Before Merge)
1. Typo in Variable Name (http_request.h:108)
std::future<bool> _http_reply_futrue = _http_reply_promise.get_future();
// ^^^^^^ should be "future"2. Hardcoded 3-Second Timeout (http_request.cpp:175)
auto status = _http_reply_futrue.wait_for(std::chrono::seconds(3));3 seconds is too aggressive for legitimate long-running loads. Consider:
- Making it configurable via
config::stream_load_async_reply_timeout_s - Default to something like 300 seconds
3. Debug Stack Trace Logging (http_request.cpp:159)
VLOG_NOTICE << "finish send reply, infos=" << infos
<< ", stack=" << get_stack_trace(); // temp locate problemget_stack_trace()is expensive even when VLOG is disabled (string is still constructed)- Comment says "temp locate problem" - should be removed before merge
Minor Issues
-
Duplicate error logging - Same error logged in both
handle()(line 122) and_send_reply()(line 157) -
Missing
finish_send_reply()insend_error()-http_channel.cppcallsfinish_send_reply()insend_reply()andsend_file()but not insend_error(). Is this intentional? -
Suggestion for
is_prepare_success- Usingshared_ptr<bool>works butstd::atomic<bool>would make the intent clearer -
Consider adding comments - The synchronization state machine with
_can_send_reply,_finish_send_reply, and the CV is correct but complex. Comments explaining the wait conditions would help future maintainers.
Thread Safety Analysis ✅
I traced through these scenarios and verified the logic is correct:
| Scenario | Result |
|---|---|
| Normal success path | ✅ Reply sent via callback |
on_header failure |
✅ Reply sent immediately, handle() skipped |
| Fragment prepare failure | ✅ Callback not invoked, handle() sends reply |
| Request timeout | ✅ _finish_send_reply=true prevents use-after-free |
Overall
Concept: ✅ Sound optimization approach
Implementation:
Thread Safety: ✅ Correct
Recommend addressing the 3 critical issues before merge.
Contributor
|
Follow-up on After deeper investigation, this is NOT a bug. Here's why:
Therefore, |
Contributor
Author
|
is_prepare_success must use std::shared_ptr |
Contributor
Author
|
run buildall |
Contributor
Author
|
run buildall |
TPC-H: Total hot run time: 31474 ms |
TPC-DS: Total hot run time: 173067 ms |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: close #53139
Problem Summary:
Release note
use stream load async return to optimize the performance under high concurrency, we treat
send_replyas callback of exec_plan_framgnet .is_prepare_successto ensure no dead lock aton_header.ctx-> _finish_send_replyensure send_reply only be invoke one time and request is valid. becuase we can't decide if callback will send_reply again when load fail or load timeout.ctx-> _can_send_replyensure send_reply only whenon_headerfail orhandlefinished, or sender will receive broken-pipe when meet data-quality errorCheck List (For Author)
Test
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)